feat/#87 slot 컴포넌트 개발 #100
Conversation
minai621
requested review from
gs0428,
ghdtjgus76 and
ShinYoung-Kim
as code owners
Walkthrough이 변경 사항은 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsError running LanguageTool: LanguageTool error: Unknown error Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
packages/primitive/components/slot/src/components/slottable.tsx (1)
3-3: 빈 인터페이스 대신 타입 별칭을 사용하는 것이 좋습니다.정적 분석 도구에서 제안한 대로, 빈 인터페이스 대신 타입 별칭을 사용하는 것이 가독성과 일관성 측면에서 더 좋습니다.
다음과 같이 변경하는 것을 제안합니다:
-export interface SlottableProps {} +export type SlottableProps = {};Tools
Biome
[error] 3-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/primitive/components/slot/src/utils/merge-props.ts (1)
22-42: 승인되었지만 console.log 문을 제거해 주세요.
mergeProps함수의 구현이 올바릅니다.하지만 34번 줄의 console.log 문은 프로덕션 환경에서는 제거되어야 합니다.
아래 diff를 적용하여 console.log 문을 제거해 주세요:
- } else if (propName === "className") { - console.log(mergeClassNames(parentValue, childValue)); - result.className = mergeClassNames(parentValue, childValue); + } else if (propName === "className") { + result.className = mergeClassNames(parentValue, childValue);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- packages/primitive/components/divider/README.md (1 hunks)
- packages/primitive/components/slot/README.md (1 hunks)
- packages/primitive/components/slot/tests/slot.test.tsx (1 hunks)
- packages/primitive/components/slot/package.json (1 hunks)
- packages/primitive/components/slot/src/components/composed-child.tsx (1 hunks)
- packages/primitive/components/slot/src/components/slottable.tsx (1 hunks)
- packages/primitive/components/slot/src/index.ts (1 hunks)
- packages/primitive/components/slot/src/slot.tsx (1 hunks)
- packages/primitive/components/slot/src/utils/composed-ref.ts (1 hunks)
- packages/primitive/components/slot/src/utils/get-element-ref.ts (1 hunks)
- packages/primitive/components/slot/src/utils/is-slottable.ts (1 hunks)
- packages/primitive/components/slot/src/utils/merge-props.ts (1 hunks)
- packages/primitive/components/slot/stories/slot.stories.tsx (1 hunks)
- packages/primitive/components/slot/tsconfig.json (1 hunks)
- packages/primitive/components/slot/tsup.config.ts (1 hunks)
- turbo/generators/templates/component/package.json.hbs (1 hunks)
Files skipped from review due to trivial changes (6)
- packages/primitive/components/divider/README.md
- packages/primitive/components/slot/package.json
- packages/primitive/components/slot/src/index.ts
- packages/primitive/components/slot/tsconfig.json
- packages/primitive/components/slot/tsup.config.ts
- turbo/generators/templates/component/package.json.hbs
Additional context used
Biome
packages/primitive/components/slot/src/components/slottable.tsx
[error] 3-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/primitive/components/slot/src/components/composed-child.tsx
[error] 14-14: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (16)
packages/primitive/components/slot/src/utils/is-slottable.ts (1)
1-7: LGTM!유틸리티 함수의 구현이 올바르게 되어 있습니다. React의
isValidElement를 사용하여 주어진 자식이 유효한 React 요소인지 확인하고, 해당 요소의 타입을Slottable컴포넌트와 비교하여Slottable컴포넌트인지 판단합니다.packages/primitive/components/slot/src/components/slottable.tsx (1)
1-11: LGTM!
Slottable컴포넌트의 구현이 올바르게 되어 있습니다. 단순히 자식 요소를 렌더링하는 역할을 수행합니다.Tools
Biome
[error] 3-3: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
packages/primitive/components/slot/src/utils/composed-ref.ts (1)
1-11: LGTM!
composeRefs유틸리티 함수의 구현이 올바르게 되어 있습니다. 여러 개의 ref를 하나의 ref 콜백으로 합성하는 역할을 수행합니다. 제공된 ref들을 순회하면서, 함수형 ref인 경우 해당 노드를 인자로 호출하고, ref 객체인 경우current속성에 노드를 할당합니다.packages/primitive/components/slot/src/utils/get-element-ref.ts (1)
1-18: 코드 변경 사항이 승인되었습니다.이 파일의 코드는 잘 구조화되어 있고 모범 사례를 따르고 있습니다. 개선이 필요한 부분은 없습니다.
packages/primitive/components/slot/src/components/composed-child.tsx (1)
1-13: 코드 변경 사항이 승인되었습니다.이 파일의 나머지 코드는 잘 구조화되어 있고 모범 사례를 따르고 있습니다. 개선이 필요한 부분은 없습니다.
Also applies to: 15-39
packages/primitive/components/slot/src/slot.tsx (1)
1-47: 코드 변경 사항이 승인되었습니다.이 파일의 코드는 잘 구조화되어 있고 모범 사례를 따르고 있습니다. 개선이 필요한 부분은 없습니다.
packages/primitive/components/slot/src/utils/merge-props.ts (3)
4-6: LGTM!코드 변경 사항이 승인되었습니다.
8-16: LGTM!코드 변경 사항이 승인되었습니다.
18-20: LGTM!코드 변경 사항이 승인되었습니다.
packages/primitive/components/slot/README.md (1)
1-151: 문서 변경 사항이 승인되었습니다!README 문서가 잘 작성되었습니다.
Slot과Slottable컴포넌트의 사용법과 주요 기능을 명확한 예제와 함께 잘 설명하고 있습니다.packages/primitive/components/slot/__tests__/slot.test.tsx (5)
7-21: 테스트 케이스가 승인되었습니다!
Slot컴포넌트의 렌더링과 ref 전달을 테스트하는 두 가지 테스트 케이스가 올바르게 구현되었습니다.
23-37: 테스트 케이스가 승인되었습니다!
Slottable컴포넌트의 렌더링을 테스트하는 테스트 케이스가 올바르게 구현되었습니다.
39-79: 테스트 케이스가 승인되었습니다!
Slot컴포넌트의 이벤트 핸들링을 테스트하는 세 가지 테스트 케이스가 올바르게 구현되었습니다.Slot컴포넌트에 이벤트 핸들러를 전달하는 경우, 자식 컴포넌트에 이벤트 핸들러를 전달하는 경우, 그리고 두 컴포넌트 모두에 이벤트 핸들러를 전달하는 경우를 잘 다루고 있습니다.
81-102: 테스트 케이스가 승인되었습니다!
Slot컴포넌트의 props 전달을 테스트하는 두 가지 테스트 케이스가 올바르게 구현되었습니다. className과 style props를Slot컴포넌트에 전달하는 시나리오를 잘 다루고 있습니다.
104-152: 테스트 케이스가 승인되었습니다!
Slot컴포넌트를 사용하는Link컴포넌트를 테스트하는 네 가지 테스트 케이스가 올바르게 구현되었습니다.asChildprop에 따라Link컴포넌트가 앵커 태그 또는Slot컴포넌트로 렌더링되는 시나리오를 잘 다루고 있습니다. 또한asChild가 true일 때Link컴포넌트의 props 전달과 이벤트 핸들링도 잘 테스트하고 있습니다.packages/primitive/components/slot/stories/slot.stories.tsx (1)
1-288: Storybook 스토리가Slot과Slottable컴포넌트의 사용법을 잘 보여주고 있습니다.이 파일은
Slot과Slottable컴포넌트를 사용하여 다양한 UI 컴포넌트(Button, Card, ListItem, FormInput)를 구현하는 방법을 비교하는 Storybook 스토리를 포함하고 있습니다. 스토리는Slot과Slottable이 어떻게 유연한 컴포넌트 구성을 만드는데 사용될 수 있는지 명확한 예시를 제공합니다.이 스토리는
Slot과Slottable컴포넌트의 기능을 문서화하고 쇼케이스하는 좋은 방법입니다. 일반적인 UI 컴포넌트를 폭넓게 다루고 있으며Slot과Slottable이 제공하는 유연성을 잘 보여줍니다. 코드는 잘 구조화되어 있고 예제는 명확하고 이해하기 쉽습니다.
packages/primitive/components/slot/src/components/composed-child.tsx
Outdated
Show resolved
Hide resolved
minai621
added
VRT
minai621
added
VRT
github-actions
bot
added
D-1
packages/primitive/components/slot/src/components/composed-child.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| export { ComposedChild }; |
There was a problem hiding this comment.
divider에서는 export default 방식으로 내보내주고 있던데 이 부분도 통일해보면 좋을 거 같습니다
There was a problem hiding this comment.
고민해봤는데, type이랑 컴포넌트를 둘 다 export 해줘야 해서 export로 대응하는게 코드상 편할 것 같습니다.
export default를 써야 하는 상황은 객체 형식으로 가져와서 사용할 때가 아니면 굳이 모듈을 전체 들고 올 필요가 없다고 생각했습니다.
There was a problem hiding this comment.
divider와 md 문서 작성하는 방식을 통일시킬 필요가 있을 것 같습니다! 아직 합의된 방식이 없어 추후 진행될 회의에서 합의하거나 해당 코드 리뷰에서 합의해야할 것 같은데 다들 어떻게 생각하세요?
@minai621 @ghdtjgus76 @gs0428
There was a problem hiding this comment.
- 엔터로 줄바꿈 vs 이어 적기
- 설치 방법 코드 개별 제공 vs 한 코드 블럭으로 제공
- md 목차
There was a problem hiding this comment.
코드리뷰가 길어질 것 같아 회의에서 논하는 것으로 하면 어떨까요?
… leftIcon와 rightIcon로 변경
- componentName을 kebabCase로 변경하여 directory 경로 수정
|
🐱 스토리북이 배포되었습니다: https://www.chromatic.com/build?appId=667edcfe9b923ba1892556f4&number=70 🐱 |
VRT 테스트 성공VRT 테스트가 성공적으로 완료되었습니다. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
packages/primitive/components/slot/README.md (3)
15-45: "Props 위임" 섹션이 잘 설명되어 있습니다.
Slot컴포넌트를 사용한 props 위임 개념이 명확하게 설명되어 있고, 코드 예제도 적절합니다.asChildprop의 사용과 스타일 병합을 잘 보여주고 있습니다.코드 예제에 간단한 주석을 추가하여 각 부분의 역할을 설명하면 더욱 이해하기 쉬울 것 같습니다. 예를 들어:
// Button 컴포넌트 정의 const Button = React.forwardRef< HTMLButtonElement, React.ButtonHTMLAttributes<HTMLButtonElement> & { asChild?: boolean } >(({ asChild, ...props }, ref) => { const Comp = asChild ? Slot : "button"; return <Comp {...props} ref={ref} />; }); // Button 컴포넌트 사용 예시 <Button asChild style={{ padding: "10px", borderRadius: "5px", background: "blue", color: "white" }} > {/* a 태그를 Button의 자식으로 사용 */} <a href="#" onClick={(e) => { e.preventDefault(); alert("Slot 버튼이 클릭되었습니다!"); }} style={{ textDecoration: "none", color: "inherit" }} > 클릭하세요 (버튼 스타일의 a 태그입니다) </a> </Button>;
47-92: "Props 병합" 섹션이 상세하고 명확합니다.
Slot컴포넌트가 부모와 자식 컴포넌트의 props를 어떻게 병합하는지 잘 설명되어 있습니다. 코드 예제와 병합 결과에 대한 주석이 이해를 돕는 데 매우 유용합니다.병합 결과를 보여주는 주석 부분을 실제 코드 블록으로 변경하면 가독성이 더 좋아질 것 같습니다. 예를 들어:
// 병합 결과: const mergedProps = { id: "child-button", "data-test": "parent", className: "child-class parent-class", onClick: chainFunctions([() => console.log('Child 클릭'), () => console.log('Parent 클릭')]), style: { padding: "10px", color: "red", backgroundColor: "blue" }, };이렇게 하면 문법 하이라이팅이 적용되어 더 읽기 쉬워집니다.
94-121: "Slottable 컴포넌트" 섹션이 잘 구성되어 있습니다.
Slottable컴포넌트의 유연한 커스터마이징 기능이 잘 설명되어 있습니다.Button컴포넌트 내에서Slottable을 사용하는 코드 예제가 적절합니다.코드 예제에서
iconLeft와iconRightprops가 사용되고 있지만,Button컴포넌트 정의에서는leftIcon과rightIcon으로 명명되어 있습니다. 일관성을 위해 이름을 통일하는 것이 좋겠습니다. 예를 들어:const Button = React.forwardRef< HTMLButtonElement, React.ButtonHTMLAttributes<HTMLButtonElement> & { asChild?: boolean; leftIcon?: React.ReactNode; rightIcon?: React.ReactNode; } >(({ asChild = false, leftIcon, rightIcon, ...props }, forwardedRef) => { const Comp = asChild ? Slot : "button"; return ( <Comp {...props} ref={forwardedRef}> {leftIcon} <Slottable>{children}</Slottable> {rightIcon} </Comp> ); }); // 사용 예시 <Button asChild leftIcon={<Icon name="link" />}> <a href="https://example.com">Visit Website</a> </Button>;packages/primitive/components/slot/stories/slot.stories.tsx (3)
32-60: 컴포넌트 정의가 잘 되어 있습니다. 작은 개선 제안이 있습니다.Button, Card, ListItem, FormInput 컴포넌트들이 일관성 있게 잘 구현되어 있습니다.
forwardRef를 사용하여 ref를 적절히 처리하고 있으며,asChildprop을 통해 렌더링 방식을 유연하게 조절할 수 있습니다.성능 최적화를 위해 각 컴포넌트의
Comp변수 선언을 메모이제이션할 수 있습니다. 예를 들어, Button 컴포넌트에서:const Button = forwardRef< HTMLButtonElement, ButtonHTMLAttributes<HTMLButtonElement> & { asChild?: boolean } >(({ asChild, ...props }, ref) => { const Comp = useMemo(() => asChild ? Slot : "button", [asChild]); return <Comp {...props} ref={ref} />; });이렇게 하면 불필요한 재렌더링을 방지할 수 있습니다.
62-116: ButtonComparison 스토리가 잘 구현되어 있습니다. 접근성 개선을 위한 제안이 있습니다.ButtonComparison 스토리가 Slot과 Slottable의 사용을 효과적으로 비교 설명하고 있습니다. 세 가지 시나리오(Slot 없음, Slot 사용, Slottable 사용)를 통해 각 접근 방식의 차이점을 잘 보여주고 있습니다.
접근성 향상을 위해 버튼에
aria-label을 추가하는 것이 좋습니다. 예를 들어:<button onClick={() => alert("클릭되었습니다!")} style={{ padding: "10px", borderRadius: "5px" }} aria-label="기본 버튼" > 클릭하세요 </button>이렇게 하면 스크린 리더 사용자들에게 더 명확한 정보를 제공할 수 있습니다.
118-291: CardComparison, ListItemComparison, FormInputComparison 스토리들이 잘 구현되어 있습니다. 일관성을 위한 작은 개선 제안이 있습니다.각 스토리가 Slot과 Slottable 컴포넌트의 유연성을 효과적으로 보여주고 있습니다. 인터랙티브 요소와 스타일링을 통해 각 접근 방식의 시각적, 기능적 차이를 명확히 제시하고 있습니다.
일관성 향상을 위해 모든 스토리에서 스타일 객체를 분리하여 정의하는 것이 좋습니다. 예를 들어:
const cardStyles = { padding: "20px", border: "1px solid #ccc", borderRadius: "10px", }; // 사용 시 <Card style={cardStyles}> {/* 내용 */} </Card>이렇게 하면 스타일의 재사용성이 높아지고, 일관된 스타일 적용이 용이해집니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
- .eslintrc.json (1 hunks)
- package.json (2 hunks)
- packages/primitive/components/slot/README.md (1 hunks)
- packages/primitive/components/slot/tests/slot.test.tsx (1 hunks)
- packages/primitive/components/slot/package.json (1 hunks)
- packages/primitive/components/slot/src/components/composed-child.tsx (1 hunks)
- packages/primitive/components/slot/src/slot.tsx (1 hunks)
- packages/primitive/components/slot/src/utils/merge-props.ts (1 hunks)
- packages/primitive/components/slot/stories/slot.stories.tsx (1 hunks)
- turbo/generators/templates/component/package.json.hbs (1 hunks)
🧰 Additional context used
🪛 Biome
packages/primitive/components/slot/src/components/composed-child.tsx
[error] 14-14: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (17)
turbo/generators/templates/component/package.json.hbs (1)
20-20: 변경 사항이 적절히 구현되었습니다.이전 리뷰 의견을 반영하여
componentName을 kebab case로 변환하도록 수정되었습니다. 이는 패키지 명명 규칙과 디렉토리 구조의 일관성을 개선하며, 템플릿의 유연성과 유지보수성을 향상시킵니다.packages/primitive/components/slot/package.json (5)
1-9: 기본 패키지 정보가 적절히 작성되었습니다.패키지 이름, 버전, 라이선스, 설명, 작성자 및 홈페이지 정보가 모두 정확하게 제공되었습니다. 이전 리뷰 코멘트에서 요청한 대로 설명이 추가되었습니다.
10-24: 패키지 구성이 적절합니다.패키지에 포함될 파일, 공개 액세스 설정, 저장소 및 이슈 트래커 정보가 올바르게 구성되어 있습니다. 이는 공개 npm 패키지에 적합한 표준 구성입니다.
25-37: 모듈 내보내기 구성이 우수합니다.ES 모듈과 CommonJS 형식을 모두 지원하도록 구성되어 있어 다양한 환경에서의 호환성이 뛰어납니다. 또한, 두 형식 모두에 대한 타입 정의가 포함되어 있어 TypeScript 사용자들에게 큰 도움이 될 것입니다.
38-41: 스크립트 구성이 적절합니다.빌드와 개발을 위한 스크립트가 잘 구성되어 있습니다. tsup을 사용하여 TypeScript 프로젝트를 빌드하는 것은 좋은 선택입니다. 개발 시 watch 모드를 사용할 수 있어 편리합니다.
42-47: 의존성 구성을 확인해주세요.React와 React DOM이 peer dependencies로 올바르게 지정되어 있습니다. 그러나 직접적인 dependencies와 devDependencies가 비어 있습니다. 이는 문제가 아닐 수 있지만, 일반적이지 않습니다.
다음 사항을 확인해주세요:
- 직접적인 dependencies가 정말로 필요하지 않은지 검토해주세요.
- devDependencies가 비어 있는 이유를 확인해주세요. 모노레포의 루트에서 개발 도구를 관리하고 있다면 문제가 없습니다.
필요한 경우 의존성을 추가하거나, 의도적으로 비워둔 것이라면 주석으로 그 이유를 설명해주시면 좋겠습니다.
packages/primitive/components/slot/src/utils/merge-props.ts (1)
4-6: isEventProp 함수 구현이 적절합니다.이 함수는 React 이벤트 핸들러 prop을 식별하는 간단하고 효율적인 방법을 제공합니다. "on"으로 시작하고 세 번째 문자가 대문자인 prop 이름을 확인하는 로직이 정확합니다.
package.json (1)
19-21: 테스트 라이브러리 추가 및 타입스크립트 버전 유지 확인
"@testing-library/jest-dom"과 "@testing-library/user-event" 라이브러리의 추가를 승인합니다. 이는 프로젝트의 테스트 기능을 향상시킬 것으로 보입니다.
타입스크립트 버전이 "^5.4.5"로 유지되고 있음을 확인했습니다. 이는 현재 변경사항에 대해 타입스크립트 주요 업데이트가 필요하지 않았음을 시사합니다.
AI 요약에서 "@testing-library/jest-dom"의 중복 항목이 제거되었다고 언급되었지만, 제공된 코드 스니펫에서는 이를 확인할 수 없습니다. 이 불일치에 대해 명확히 해주시겠습니까?
중복 항목 제거에 대한 언급을 확인하기 위해 다음 스크립트를 실행해 주세요:
이 스크립트는 이전 버전의 package.json 파일에서 "@testing-library/jest-dom"의 발생을 확인합니다. 결과에 따라 중복 항목의 존재 여부를 판단할 수 있습니다.
Also applies to: 44-44
.eslintrc.json (1)
74-79: 스토리북 파일에 대한 ESLint 규칙 변경이 적절해 보입니다.이 변경사항은
*.stories.tsx파일에 대해react/display-name규칙을 비활성화합니다. 이는 스토리북 컴포넌트에서 종종 익명 컴포넌트를 사용하거나 명시적인 디스플레이 이름이 필요하지 않기 때문에 적절한 변경으로 보입니다. 이 변경으로Slot컴포넌트의 스토리북 예제 작성이 더 용이해질 것입니다.packages/primitive/components/slot/README.md (3)
1-13: README 파일의 소개 및 설치 섹션이 명확하고 포괄적입니다.소개 부분에서
slot컴포넌트의 목적과 이점을 잘 설명하고 있습니다. 설치 지침도 다양한 패키지 관리자에 대해 제공되어 있어 사용자 친화적입니다.
123-143: "Ref 핸들링" 섹션이 명확하고 유용합니다.
Slot컴포넌트가 ref를 어떻게 처리하는지 잘 설명되어 있습니다. 코드 예제에서useRef와useEffect를 사용하여 ref 전달과 사용 방법을 적절히 보여주고 있습니다.이 섹션은
Slot컴포넌트의 고급 기능을 이해하는 데 도움이 될 것입니다.
1-143: 전반적으로 우수한 README 파일입니다.이 README 파일은
@warrr-ui/primitive-slot패키지의 모든 핵심 측면을 포괄적으로 다루고 있습니다. 각 섹션은 명확한 설명과 관련 코드 예제를 제공하여 사용자가 패키지를 이해하고 사용하는 데 큰 도움이 될 것입니다.구조가 잘 잡혀 있고 정보가 풍부하며, 코드 예제들이 실제 사용 사례를 잘 보여주고 있습니다. 제안된 몇 가지 작은 개선 사항들을 반영하면 더욱 완벽한 문서가 될 것 같습니다.
packages/primitive/components/slot/stories/slot.stories.tsx (2)
1-30: LGTM: 임포트와 메타 객체 정의가 적절합니다.임포트 문과 메타 객체 정의가 잘 구성되어 있습니다. Storybook에 필요한 정보를 적절히 제공하고 있으며, 컴포넌트 설명과
childrenprop에 대한 설명도 포함되어 있습니다.
1-291: 전반적으로 우수한 품질의 Storybook 스토리 파일입니다.이 파일은 Slot 컴포넌트의 기능을 다양한 예시를 통해 효과적으로 보여주고 있습니다. 코드 구조가 명확하고 일관성 있게 작성되어 있어 가독성과 유지보수성이 뛰어납니다. 각 스토리는 Slot과 Slottable 컴포넌트의 사용 사례를 잘 설명하고 있으며, 인터랙티브 요소를 통해 사용자가 쉽게 이해할 수 있도록 구성되어 있습니다.
제안된 개선 사항들(메모이제이션, 접근성, 스타일 분리)을 적용하면 코드의 품질을 더욱 향상시킬 수 있을 것입니다. 전체적으로 이 파일은 Slot 컴포넌트의 기능과 유연성을 잘 보여주는 우수한 예시 자료입니다.
packages/primitive/components/slot/src/components/composed-child.tsx (1)
16-35: 구현이 적절합니다.
ComposedChild컴포넌트는 props와 ref를 적절히 처리하여 자식 요소를 확장하고 있습니다.packages/primitive/components/slot/src/slot.tsx (1)
26-28: 타입 단언 없이도 타입 추론이 가능합니다.
isSlottable함수가 ReactElement에 대한 타입 가드 역할을 하므로,(newElement as ReactElement)와 같은 타입 단언 없이도 타입이 올바르게 추론될 수 있습니다.packages/primitive/components/slot/__tests__/slot.test.tsx (1)
27-42:Slottable컴포넌트에 대한 테스트가 적절하게 작성되었습니다
Slottable컴포넌트가 올바르게 렌더링되고 있는지 확인하는 테스트가 정확합니다.
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| type AnyProps = Record<string, any>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
eslint-disable 주석 대신 더 구체적인 접근 방식을 고려해 보세요.
현재 파일 전체에 대해 @typescript-eslint/no-explicit-any 규칙을 비활성화하고 있습니다. 이는 타입 안전성을 저해할 수 있습니다.
다음과 같은 대안을 고려해 보세요:
- 가능한 경우
any대신 더 구체적인 타입을 사용합니다. any를 사용해야 하는 특정 라인에 대해서만 규칙을 비활성화합니다.- 프로젝트의
.eslintrc파일에 이 규칙에 대한 예외를 추가합니다.
예를 들어, AnyProps 타입에 대해서만 규칙을 비활성화하려면 다음과 같이 할 수 있습니다:
// eslint-disable-next-line @typescript-eslint/no-explicit-any
type AnyProps = Record<string, any>;이렇게 하면 필요한 곳에서만 any를 사용하고 나머지 코드에서는 타입 안전성을 유지할 수 있습니다.
| function chainFunctions(...callbacks: any[]) { | ||
| return (...args: any[]) => { | ||
| callbacks.forEach((callback) => { | ||
| if (typeof callback === "function") { | ||
| callback(...args); | ||
| } | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
chainFunctions의 타입 안전성을 개선해 보세요.
현재 구현은 여러 콜백을 효과적으로 체인화하고 있습니다. 하지만 any 타입의 사용으로 인해 타입 안전성이 떨어집니다.
다음과 같이 제네릭을 사용하여 타입 안전성을 개선할 수 있습니다:
function chainFunctions<T extends any[]>(...callbacks: Array<((...args: T) => void) | undefined>): (...args: T) => void {
return (...args: T) => {
callbacks.forEach((callback) => {
if (typeof callback === "function") {
callback(...args);
}
});
};
}이 방식을 사용하면 콜백 함수의 인자 타입을 보존하면서도 undefined를 허용하여 유연성을 유지할 수 있습니다.
| function mergeClassNames(...classNames: any[]): string { | ||
| return classNames.filter(Boolean).join(" "); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
mergeClassNames 함수의 타입 안전성과 견고성을 개선해 보세요.
현재 구현은 클래스 이름을 효과적으로 병합하고 있습니다. 하지만 any 타입의 사용으로 인해 타입 안전성이 떨어지며, 일부 edge case를 고려하지 않고 있습니다.
다음과 같이 개선할 수 있습니다:
function mergeClassNames(...classNames: Array<string | undefined | null | false>): string {
return classNames.filter((className): className is string =>
typeof className === 'string' && className.trim() !== ''
).join(" ");
}이 개선된 버전은:
- 허용되는 타입을 명시적으로 정의하여 타입 안전성을 높입니다.
- 빈 문자열을 필터링하여 불필요한 공백을 방지합니다.
- 타입 가드를 사용하여 TypeScript가 필터링 후 문자열 배열임을 인식하도록 합니다.
| export function mergeProps(parentProps: AnyProps, childProps: AnyProps): AnyProps { | ||
| const result = { ...parentProps }; | ||
|
|
||
| for (const propName in childProps) { | ||
| const parentValue = parentProps[propName]; | ||
| const childValue = childProps[propName]; | ||
|
|
||
| if (isEventProp(propName)) { | ||
| result[propName] = chainFunctions(parentValue, childValue); | ||
| } else if (propName === "style") { | ||
| result.style = { ...parentValue, ...childValue }; | ||
| } else if (propName === "className") { | ||
| result.className = mergeClassNames(parentValue, childValue); | ||
| } else { | ||
| result[propName] = childValue; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
mergeProps 함수의 타입 안전성과 불변성을 개선해 보세요.
현재 구현은 부모와 자식 컴포넌트의 props를 효과적으로 병합하고 있습니다. 하지만 any 타입의 사용으로 인해 타입 안전성이 떨어지며, 결과 객체를 직접 변경하고 있습니다.
다음과 같은 개선을 고려해 보세요:
- 제네릭을 사용하여 타입 안전성 개선:
export function mergeProps<P extends Record<string, unknown>, C extends Record<string, unknown>>(
parentProps: P,
childProps: C
): P & C {
const result = { ...parentProps } as P & C;
// ... (나머지 로직)
return result;
}- Object.assign 또는 스프레드 연산자를 사용하여 불변성 유지:
if (propName === "style") {
result.style = { ...result.style, ...childValue };
} else {
result = { ...result, [propName]: childValue };
}- 타입 가드를 사용하여 더 정확한 타입 체크:
if (typeof parentValue === 'function' && typeof childValue === 'function') {
result[propName as keyof (P & C)] = chainFunctions(parentValue, childValue);
}이러한 변경을 통해 타입 안전성이 향상되고, 불변성이 보장되며, 더 견고한 코드를 작성할 수 있습니다.
| }); | ||
| } | ||
|
|
||
| return Children.count(children) > 1 ? Children.only(null) : null; |
There was a problem hiding this comment.
Children.only(null)의 사용에 문제가 있습니다
Children.only는 하나의 유효한 React 자식 요소를 기대하며, null을 전달하면 오류가 발생합니다. 자식이 둘 이상인 경우 적절한 오류 처리를 하는 것이 좋습니다.
다음과 같이 수정하는 것을 제안합니다:
-return Children.count(children) > 1 ? Children.only(null) : null;
+if (Children.count(children) > 1) {
+ throw new Error("ComposedChild 컴포넌트는 하나의 자식만 받아야 합니다.");
+}
+return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Children.count(children) > 1 ? Children.only(null) : null; | |
| if (Children.count(children) > 1) { | |
| throw new Error("ComposedChild 컴포넌트는 하나의 자식만 받아야 합니다."); | |
| } | |
| return null; |
| const Slot = forwardRef<HTMLElement, SlotProps>((props, ref) => { | ||
| const { children, ...restProps } = props; | ||
| const childrenArray = Children.toArray(children); | ||
| const slottable = childrenArray.find(isSlottable); | ||
|
|
||
| if (slottable) { | ||
| const newElement = slottable.props.children; | ||
|
|
||
| if (isValidElement(newElement)) { | ||
| const newChildren = childrenArray.map((child) => | ||
| child === slottable ? (newElement as ReactElement).props.children : child | ||
| ); | ||
|
|
||
| return ( | ||
| <ComposedChild {...restProps} ref={ref}> | ||
| {React.cloneElement(newElement, undefined, newChildren)} | ||
| </ComposedChild> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <ComposedChild {...restProps} ref={ref}> | ||
| {children} | ||
| </ComposedChild> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
ComposedChild 컴포넌트에서 ref가 실제 DOM 요소에 전달되지 않습니다.
ComposedChild가 forwardRef로 정의되어 있으나, 내부에서 ref를 실제 DOM 요소에 할당하지 않고 있습니다. 이로 인해 부모 컴포넌트에서 전달한 ref가 제대로 작동하지 않습니다.
컴포넌트 내부에서 ref를 원하는 DOM 요소에 할당하도록 수정해 주세요.
🔗 Analysis chain
ComposedChild 컴포넌트로의 ref 전달을 확인해주세요.
Slot 컴포넌트에서 ref를 <ComposedChild>에게 전달하고 있습니다. ComposedChild가 이 ref를 올바르게 처리하고 전달하는지 확인이 필요합니다.
다음 스크립트를 실행하여 ComposedChild가 ref를 제대로 전달하고 있는지 확인하세요:
이전 스크립트에서 tsx 파일 유형이 인식되지 않아 실행에 실패했습니다. ComposedChild가 forwardRef를 사용하여 ref를 전달하고 있는지 확인하기 위해 다음 스크립트를 실행해 주세요:
ComposedChild가 forwardRef를 사용하여 ref를 올바르게 전달하고 있는지 추가로 확인하기 위해 다음 스크립트를 실행해 주세요:
이전 스크립트가 패턴 오류로 실패했습니다. ref가 ComposedChild 내의 실제 DOM 요소에 전달되고 있는지 확인하기 위해 다음 스크립트를 실행해 주세요:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: ComposedChild 컴포넌트가 forwardRef를 사용하여 ref를 전달하는지 확인합니다.
# Expected Result: ComposedChild는 forwardRef로 정의되어 있어야 합니다.
# ComposedChild 정의에서 forwardRef 사용 여부 확인
rg --type tsx -A 5 $'const ComposedChild = forwardRef'
Length of output: 83
Script:
#!/bin/bash
# Description: ComposedChild 컴포넌트가 forwardRef를 사용하여 ref를 전달하는지 확인합니다.
# Expected Result: ComposedChild는 forwardRef로 정의되어 있어야 합니다.
# ComposedChild 정의에서 forwardRef 사용 여부 확인
rg "const ComposedChild = forwardRef" --glob "*.tsx"
Length of output: 216
Script:
#!/bin/bash
# Description: ComposedChild 내부에서 ref가 실제 DOM 요소에 전달되는지 확인합니다.
# Expected Result: ref가 DOM 요소에 할당되어야 합니다.
# ComposedChild 파일에서 ref가 DOM 요소에 전달되는지 확인
ast-grep --lang typescript --pattern $'forwardRef($_, $_) { $$$ return (<div ref={$_} />); $$$ }' packages/primitive/components/slot/src/components/composed-child.tsx
Length of output: 647
Script:
#!/bin/bash
# Description: ComposedChild 컴포넌트 내부에서 ref가 실제 DOM 요소에 전달되는지 확인합니다.
# Expected Result: JSX 요소에 ref 속성이 할당되어 있어야 합니다.
# ComposedChild 파일에서 ref 속성 사용 여부 확인
rg 'ref\s*=' packages/primitive/components/slot/src/components/composed-child.tsx
Length of output: 83
| import { ComponentProps, createRef } from "react"; | ||
|
|
||
| import { render, screen } from "@testing-library/react"; | ||
| import { userEvent } from "@testing-library/user-event"; |
There was a problem hiding this comment.
userEvent 모듈의 올바른 임포트 필요
userEvent는 디폴트 내보내기이므로 중괄호 없이 임포트해야 합니다. 그렇지 않으면 테스트에서 userEvent의 메서드를 사용할 때 오류가 발생할 수 있습니다.
- import { userEvent } from "@testing-library/user-event";
+ import userEvent from "@testing-library/user-event";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { userEvent } from "@testing-library/user-event"; | |
| import userEvent from "@testing-library/user-event"; |
예상 동작 (Expected Behavior)
Slot컴포넌트를 사용하여 부모 컴포넌트와 자식 컴포넌트 간의 관계를 재정의하거나 스타일을 변경할 수 있습니다.부모 컴포넌트에서 전달한 props와 자식 컴포넌트에서 정의한 props가 자동으로 병합되어 적용됩니다.
Slottable컴포넌트를 사용하여 컴포넌트의 특정 부분을 유연하게 대체할 수 있습니다.상위 컴포넌트에서 전달된 ref는
Slot컴포넌트에 의해 처리되어 슬롯에 전달된 요소의 ref와 병합됩니다.고민했던 내용 (Considerations)
안쓰는 interface들은 삭제할지 고민을 많이 했는데, forwardRef나 다른 타입과의 상호작용이 생길 수 있어 그냥 빈타입으로 생성하였습니다. (어차피 js 번들에는 안들어가서 빈 채로 두는게 가독성이 더 좋은 것 같습니다)
Slot 컴포넌트의 사용 방법에 대해 이해하기 어려울 것 같아 storybook에 예시를 최대한 많이 작성하였으니 참고 부탁드립니다.
radix-ui slot
render delegation에 대한 블로그 게시글
관련 이슈
Summary by CodeRabbit
New Features
@warrr-ui/divider에서@warrr-ui/primitive-divider로 변경되었습니다.Slot및Slottable컴포넌트가 새롭게 추가되었습니다.Slot컴포넌트가 자식 컴포넌트를 유연하게 관리하고 렌더링하는 기능을 제공합니다.Slot컴포넌트에 대한 Storybook 스토리가 추가되었습니다.Link컴포넌트가 추가되어,Slot또는 앵커 태그로 렌더링할 수 있습니다.Bug Fixes
Documentation
@warrr-ui/primitive-slot패키지에 대한 문서가 추가되었습니다.Slot컴포넌트의 사용법과 예시가 포함된 스토리가 추가되었습니다.